-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Removed JUnit5 dependency for ClientTpcConfigTest #26265
base: master
Are you sure you want to change the base?
Conversation
…5) using org.junit.Assert.assertTrue (JUnit4);
Can one of the admins verify this patch? |
2 similar comments
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Internal PR hazelcast/hazelcast-mono#903 |
@dr-divago thanks for your contribution! It's great you've addressed these issues - had you considered amending the test referenced in #26193 to see if further examples could be caught going forwards? |
@JackPGreen Do you mean to check all the references of JUnit5 in the codebase? Or fixing |
I thought fixing the |
… and Junit5 checks
Hi @JackPGreen |
The job Click to expand the log file-------------------------- ---------SUMMARY---------- -------------------------- [ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.3.1:checkstyle (default) on project hazelcast-archunit-rules: An error has occurred in Checkstyle report generation. Failed during checkstyle execution: There are 2 errors reported by Checkstyle 9.3 with /home/jenkins/jenkins_slave/workspace/Hazelcast-mono-OS-pr-builder_2/hazelcast/checkstyle/checkstyle.xml ruleset. -> [Help 1] -------------------------- ---------ERRORS----------- -------------------------- [ERROR] /home/jenkins/jenkins_slave/workspace/Hazelcast-mono-OS-pr-builder_2/hazelcast/hazelcast-archunit-rules/src/main/java/com/hazelcast/test/archunit/MixTestAnnotationsCondition.java:24:17: Using the '.*' form of import should be avoided - org.junit.*. [AvoidStarImport] -------------------------- [ERROR] /home/jenkins/jenkins_slave/workspace/Hazelcast-mono-OS-pr-builder_2/hazelcast/hazelcast-archunit-rules/src/main/java/com/hazelcast/test/archunit/MixTestAnnotationsCondition.java:39:59: Name 'JUNIT_4_ClASS_ANNOTATION' must match pattern '^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$'. [ConstantName] -------------------------- [ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.3.1:checkstyle (default) on project hazelcast-archunit-rules: An error has occurred in Checkstyle report generation. Failed during checkstyle execution: There are 2 errors reported by Checkstyle 9.3 with /home/jenkins/jenkins_slave/workspace/Hazelcast-mono-OS-pr-builder_2/hazelcast/checkstyle/checkstyle.xml ruleset. -> [Help 1] -------------------------- [ERROR] -------------------------- [ERROR] Re-run Maven using the -X switch to enable full debug logging. -------------------------- [ERROR] -------------------------- [ERROR] For more information about the errors and possible solutions, please read the following articles: -------------------------- [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException -------------------------- [ERROR] -------------------------- [ERROR] After correcting the problems, you can resume the build with the command -------------------------- [ERROR] mvn -rf :hazelcast-archunit-rules -------------------------- |
… and Junit5 checks
Hi @JackPGreen |
It's an internal tool unfortunately, but all tests have passed :) |
private boolean hasJUnit5Assertions(JavaClass item) { | ||
Set<JavaMethodCall> methodCalls = item.getMethodCallsFromSelf(); | ||
return methodCalls.stream().anyMatch(call -> call.getTarget().getFullName().contains("org.junit.jupiter.api.Assertions")); | ||
} | ||
private boolean hasJUnit4Assertions(JavaClass item) { | ||
Set<JavaMethodCall> methodCalls = item.getMethodCallsFromSelf(); | ||
return methodCalls.stream().anyMatch(call -> call.getTarget().getFullName().contains("org.junit.Assert")); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - it's possible to centralise the logic into a single method that these can delegate to with different fullName
parameters
private boolean hasAnyJUnit4Annotations(JavaClass item) { | ||
boolean hasJUnit4Annotation = item.getMethods().stream() | ||
.anyMatch(method -> JUNIT_4_ANNOTATION_CLASSES.stream() | ||
.anyMatch(method::isAnnotatedWith)); | ||
|
||
boolean hasJUnit4ClassAnnotation = item.getAnnotations().stream() | ||
.anyMatch(annotation -> JUNIT_4_CLASS_ANNOTATION.stream() | ||
.anyMatch(aa -> annotation.getRawType().isAssignableFrom(aa))); | ||
|
||
return hasJUnit4Annotation || hasJUnit4ClassAnnotation; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
} | ||
private boolean hasAnyJUnit4Annotations(JavaClass item) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | |
private boolean hasAnyJUnit4Annotations(JavaClass item) { | |
} | |
private boolean hasAnyJUnit4Annotations(JavaClass item) { |
Add some space between methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for finding and fixing these!
|
||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need these extra blank lines?
Hi @JackPGreen is something missing in the PR? |
No, just waiting for a second reviewer. |
This pull request has been closed because it was already closed as https://github.com/hazelcast/hazelcast-mono/pull/903 |
@JackPGreen why the PR was closed? |
It’s open again now, apologies :) |
@@ -20,8 +20,8 @@ | |||
import com.hazelcast.test.HazelcastTestSupport; | |||
import com.hazelcast.test.annotation.ParallelJVMTest; | |||
import com.hazelcast.test.annotation.QuickTest; | |||
import org.junit.Test; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this form it won't work:
- Junit 4 test must be public
- We use also ParameterizedTest in this class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, I miss it. Do you suggest rewrite the test with Parameterized runner?
Removed JUnit5 dependency for ClientTpcConfigTest
Fixes #26193
Checklist:
Team:
,Type:
,Source:
,Module:
) and Milestone setAdd to Release Notes
label if changes should be mentioned in release notes orNot Release Notes content
if changes are not relevant for release notes@Nonnull/@Nullable
annotations@since
tags in Javadoc